wow @codesuki I ❤️ so much. Thanks!

I cloned this locally and I'm trying to get the devserver to work. Both at your commit and the one I've pushed to your branch, I can
require(['react']) in the devtools, but require(['react']).default is undefined, so the generated js for greeting.tsx

$ cat dist/bin/greeting.js 
(function (factory) {
    if (typeof module === "object" && typeof module.exports === "object") {
        var v = factory(require, exports);
        if (v !== undefined) module.exports = v;
    }
    else if (typeof define === "function" && define.amd) {
        define("examples_reactjs/greeting", ["require", "exports", "react"], factory);
    }
})(function (require, exports) {
    "use strict";
    Object.defineProperty(exports, "__esModule", { value: true });
    const react_1 = require("react");
    const greeting = react_1.default.createElement("h1", null, "Hello, world");

fails there at that last line.

Does it have anything to do with the tsxFactory option to tsc? I'll keep debugging but LMK if you already have an answer. I assume the devserver worked for you?

Hey, thanks for improving the PR! It's much cleaner now. I didn't know what to do about the tests.

I just left for Hawaii vacation so I cannot check the code now.

I guess you get a null pointer exception?
When I prepared the PR both the dev server and the prod server (bundle) worked. I don't remember if my output also had 'react.default'. I'll confirm once I am back.

Thanks for working on these rules!

devserver works for me. I got different output from @alexeagle. TypeScript version is not same?

$ cat dist/bin/greeting.js                                                                                                                                                                                                                                              git:react-example*
(function (factory) {
    if (typeof module === "object" && typeof module.exports === "object") {
        var v = factory(require, exports);
        if (v !== undefined) module.exports = v;
    }
    else if (typeof define === "function" && define.amd) {
        define("examples_reactjs/greeting", ["require", "exports", "react"], factory);
    }
})(function (require, exports) {
    "use strict";
    Object.defineProperty(exports, "__esModule", { value: true });
    const React = require("react");
    const greeting = React.createElement("h1", null, "Hello, world");
    exports.default = greeting;
});

Okay, problem here was

• for devmode to work properly, we should use import * as React from 'react' so that we have output JS with react_1.default.createElement rather than react_1.createElement
• that means that the rollup-plugin-commonjs wants to find the createElement symbol in the react/index.js file - but it can't. Error: 'render' is not exported by node_modules/react-dom/index.js rollup/rollup-plugin-node-resolve#58 describes the problem

I've added another commit here to make the test pass - but we don't really want the namedExports: { 'bazel-out/k8-fastbuild/bin/bundle.es6/external/npm/node_modules/react/index.js': ['createElement'], mumble in the rollup.config.js

Let's say you're not using Bazel. It seems that bundling React into your app with Rollup requires this workaround where you list some symbols in the rollup.config. Seems not desirable.
So I suspect we should change this example to not bundle React into the app, instead we should load it as a script at runtime?

Does the bundle work when using import * as React from 'react'?
I ran into this issue before: #933

Ah, the named exports you mention fixes this right? I agree, having to adjust rollup.config.js is cumbersome.

Anyhow, what is the benefit of using import * over import React from ...?
i.e. what is the difference between react_1.default.createElement and react_1.createElement?

I haven't done any web dev for several years now so I can't say anything useful to whether or not react should be bundled or loaded at runtime. Correct me if I say something wrong.
In general, I think loading it as an external dep would allow the browser to cache it between pages if the same version from some CDN is referenced, on the other hand, bundling could enable dead code elimination which I am not sure rollup actually does.

For our actual production case, we don't mind either way I think. (I know this is just about the example here)

@codesuki @alexeagle thanks for your work on this!

Any thoughts on how this may work with a CRA application?

Hey - this is really cool! Some (JS/node) stylistic notes:

1. It may be more cleaner to teach the rollup rule about --environment=NODE_ENV:production. Potentially even setting it by default. If we don't, we'll always risk that we bundle suboptimal 3rd party code. The "it throws" is the happy path - the worse case is that the check is protected by defensive code and it's just silently running verbose dev code in production.
2. Afaik import React from 'react' is the canonical import and at least in rollup it does work. The big issue is that react ships CommonJS, not ES modules. So import * isn't really correct since the only namespace member from an ESM perspective is default, there are no named exports.
3. In the spec, it may make sense to use a clean sandbox for the DOM APIs instead of putting them into the global scope. Here's an example how it might look like:

'use strict';

const domino = require('domino');
const fs = require('fs');
const vm = require('vm');

describe('react webapp', () => {
  let sandbox;
  beforeAll(() => {
    // Domino gives us enough of the DOM API that we can run our JavaScript in node rather than the
    // browser. That makes this test a lot faster
    const html = fs.readFileSync(require.resolve('./index.html'));
    const window = domino.createWindow(html, '/');
    // Make all Domino types available as types in the sandbox.
    sandbox = vm.createContext({
      window,
      document: window.document,
      navigator: window.navigator,
      process: {
        env: 'production',
      },
    });
  });

  it('works', () => {
    const bundlePath = require.resolve('./bundle.es2015');
    const bundleContents = fs.readFileSync(bundlePath, 'utf8');
    vm.runInContext(bundleContents, sandbox, { filename: bundlePath });

    expect(sandbox.document.body.textContent.trim()).toEqual('Hello from React!');
  });
});

Lets wait till the new rollup rule has landed, there's a couple of different rollup configs/plugins I'd like to try out here which may make things a bit easier, which will be possible with the new rule.

@jkrems On your point 3. while I fundamentally agree that sandboxing code is a good idea, I don't think it fits unit tests.
None of the React examples i've seen do it, it'll make unit tests slower and it's bad UX since there's a bunch of API's not known to most devs.
I don't think we should recommend that approach for unit testing in this example.

We tried following this approach and it worked OK for getting react working with a ts_devserver, but when trying to use react-router it has problems. Both react-router and react-router-dom have a check for the type of module they are loaded with (to make sure it's the same). Even if both are loaded in the ts_devserver using __umd, they report as different.

It would be helpful if the example included some of these common libraries, since they seem to be sensitive to how things are connected. Specifically, when looking in the browser:

react-router.umd.js includes

if (process.env.NODE_ENV !== "production") {
  if (typeof window !== "undefined") {
    var global = window;
    var key = "__react_router_build__";
    var buildNames = {
      cjs: "CommonJS",
      esm: "ES modules",
      umd: "UMD"
    };

    if (global[key] && global[key] !== "esm") {
      var initialBuildName = buildNames[global[key]];
      var secondaryBuildName = buildNames["esm"]; // TODO: Add link to article that explains in detail how to avoid
      // loading 2 different builds.

      throw new Error("You are loading the " + secondaryBuildName + " build of React Router " + ("on a page that is already running the " + initialBuildName + " ") + "build, so things won't work right.");
    }

    global[key] = "esm";
  }
}

and react-router-dom.umd.js includes:

{
  if (typeof window !== "undefined") {
    var global = window;
    var key = "__react_router_build__";
    var buildNames = {
      cjs: "CommonJS",
      esm: "ES modules",
      umd: "UMD"
    };

    if (global[key] && global[key] !== "cjs") {
      var initialBuildName = buildNames[global[key]];
      var secondaryBuildName = buildNames["cjs"]; // TODO: Add link to article that explains in detail how to avoid
      // loading 2 different builds.

      throw new Error("You are loading the " + secondaryBuildName + " build of React Router " + ("on a page that is already running the " + initialBuildName + " ") + "build, so things won't work right.");
    }

    global[key] = "cjs";
  }
}

On your point 3. while I fundamentally agree that sandboxing code is a good idea, I don't think it fits unit tests.

What I wrote is pretty much how Jest works. It hides it behind some nice facade but this is pretty close to the exact behavior (down to the node APIs used). It's not very common to do this setup in individual test files, that's definitely true. :) But if your test is using document or window (as opposed to shallow renders via sth like enzyme) you absolutely shouldn't be setting those on the "normal" global object.

We have just checkouted your branch and we have a runtime error.

@alexeagle I think this can be closed now

@mrmeku why is that?

FYI bazel-out/k8-fastbuild/bin/bundle.es6/external/npm/node_modules/react/index.js doesn't work on macos, the path would be bazel-out/darwin-fastbuild/bin/bundle.es6/external/npm/node_modules/react/index.js

This Pull Request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs in two weeks. Collaborators can add a "cleanup" or "need: discussion" label to keep it open indefinitely. Thanks for your contributions to rules_nodejs!

This PR was automatically closed because it went two weeks without a reply since it was labeled "Can Close?"